-
Notifications
You must be signed in to change notification settings - Fork 311
Stable Config improvements #9259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 13 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.045 s) : 0, 1044528
Total [baseline] (8.591 s) : 0, 8591427
Agent [candidate] (1.042 s) : 0, 1041818
Total [candidate] (8.568 s) : 0, 8568090
section iast
Agent [baseline] (1.177 s) : 0, 1176743
Total [baseline] (9.318 s) : 0, 9317815
Agent [candidate] (1.175 s) : 0, 1175315
Total [candidate] (9.308 s) : 0, 9308486
gantt
title insecure-bank - break down per module: candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.422 ms) : 0, 1422
crashtracking [candidate] (1.427 ms) : 0, 1427
BytebuddyAgent [baseline] (730.962 ms) : 0, 730962
BytebuddyAgent [candidate] (729.466 ms) : 0, 729466
GlobalTracer [baseline] (241.78 ms) : 0, 241780
GlobalTracer [candidate] (241.92 ms) : 0, 241920
AppSec [baseline] (30.124 ms) : 0, 30124
AppSec [candidate] (30.04 ms) : 0, 30040
Debugger [baseline] (6.011 ms) : 0, 6011
Debugger [candidate] (6.05 ms) : 0, 6050
Remote Config [baseline] (652.874 µs) : 0, 653
Remote Config [candidate] (657.711 µs) : 0, 658
Telemetry [baseline] (12.586 ms) : 0, 12586
Telemetry [candidate] (11.325 ms) : 0, 11325
section iast
crashtracking [baseline] (1.426 ms) : 0, 1426
crashtracking [candidate] (1.428 ms) : 0, 1428
BytebuddyAgent [baseline] (849.418 ms) : 0, 849418
BytebuddyAgent [candidate] (848.743 ms) : 0, 848743
GlobalTracer [baseline] (232.346 ms) : 0, 232346
GlobalTracer [candidate] (232.981 ms) : 0, 232981
AppSec [baseline] (25.972 ms) : 0, 25972
AppSec [candidate] (27.8 ms) : 0, 27800
Debugger [baseline] (8.478 ms) : 0, 8478
Debugger [candidate] (5.784 ms) : 0, 5784
Remote Config [baseline] (581.482 µs) : 0, 581
Remote Config [candidate] (589.455 µs) : 0, 589
Telemetry [baseline] (8.272 ms) : 0, 8272
Telemetry [candidate] (8.028 ms) : 0, 8028
IAST [baseline] (29.223 ms) : 0, 29223
IAST [candidate] (29.013 ms) : 0, 29013
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.044 s) : 0, 1043627
Total [baseline] (10.637 s) : 0, 10637212
Agent [candidate] (1.051 s) : 0, 1051072
Total [candidate] (10.749 s) : 0, 10748718
section appsec
Agent [baseline] (1.222 s) : 0, 1221675
Total [baseline] (10.721 s) : 0, 10721273
Agent [candidate] (1.22 s) : 0, 1219993
Total [candidate] (10.786 s) : 0, 10785776
section iast
Agent [baseline] (1.197 s) : 0, 1196773
Total [baseline] (10.96 s) : 0, 10959574
Agent [candidate] (1.197 s) : 0, 1196723
Total [candidate] (11.061 s) : 0, 11061203
section profiling
Agent [baseline] (1.203 s) : 0, 1202842
Total [baseline] (10.968 s) : 0, 10968215
Agent [candidate] (1.194 s) : 0, 1194359
Total [candidate] (10.898 s) : 0, 10898283
gantt
title petclinic - break down per module: candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.442 ms) : 0, 1442
crashtracking [candidate] (1.509 ms) : 0, 1509
BytebuddyAgent [baseline] (730.435 ms) : 0, 730435
BytebuddyAgent [candidate] (737.724 ms) : 0, 737724
GlobalTracer [baseline] (241.385 ms) : 0, 241385
GlobalTracer [candidate] (243.73 ms) : 0, 243730
AppSec [baseline] (30.013 ms) : 0, 30013
AppSec [candidate] (30.361 ms) : 0, 30361
Debugger [baseline] (6.011 ms) : 0, 6011
Debugger [candidate] (6.078 ms) : 0, 6078
Remote Config [baseline] (642.13 µs) : 0, 642
Remote Config [candidate] (660.538 µs) : 0, 661
Telemetry [baseline] (12.706 ms) : 0, 12706
Telemetry [candidate] (9.794 ms) : 0, 9794
section appsec
crashtracking [baseline] (1.44 ms) : 0, 1440
crashtracking [candidate] (1.434 ms) : 0, 1434
BytebuddyAgent [baseline] (754.182 ms) : 0, 754182
BytebuddyAgent [candidate] (753.311 ms) : 0, 753311
GlobalTracer [baseline] (235.301 ms) : 0, 235301
GlobalTracer [candidate] (234.685 ms) : 0, 234685
IAST [baseline] (23.455 ms) : 0, 23455
IAST [candidate] (23.502 ms) : 0, 23502
AppSec [baseline] (167.73 ms) : 0, 167730
AppSec [candidate] (168.436 ms) : 0, 168436
Debugger [baseline] (7.999 ms) : 0, 7999
Debugger [candidate] (8.744 ms) : 0, 8744
Remote Config [baseline] (631.882 µs) : 0, 632
Remote Config [candidate] (613.686 µs) : 0, 614
Telemetry [baseline] (9.894 ms) : 0, 9894
Telemetry [candidate] (8.226 ms) : 0, 8226
section iast
crashtracking [baseline] (1.454 ms) : 0, 1454
crashtracking [candidate] (1.45 ms) : 0, 1450
BytebuddyAgent [baseline] (865.587 ms) : 0, 865587
BytebuddyAgent [candidate] (865.122 ms) : 0, 865122
GlobalTracer [baseline] (235.486 ms) : 0, 235486
GlobalTracer [candidate] (235.086 ms) : 0, 235086
IAST [baseline] (28.635 ms) : 0, 28635
IAST [candidate] (28.003 ms) : 0, 28003
AppSec [baseline] (28.676 ms) : 0, 28676
AppSec [candidate] (29.369 ms) : 0, 29369
Debugger [baseline] (6.566 ms) : 0, 6566
Debugger [candidate] (7.479 ms) : 0, 7479
Remote Config [baseline] (592.864 µs) : 0, 593
Remote Config [candidate] (602.542 µs) : 0, 603
Telemetry [baseline] (8.5 ms) : 0, 8500
Telemetry [candidate] (8.371 ms) : 0, 8371
section profiling
crashtracking [baseline] (1.417 ms) : 0, 1417
crashtracking [candidate] (1.41 ms) : 0, 1410
BytebuddyAgent [baseline] (766.303 ms) : 0, 766303
BytebuddyAgent [candidate] (761.072 ms) : 0, 761072
GlobalTracer [baseline] (223.187 ms) : 0, 223187
GlobalTracer [candidate] (221.773 ms) : 0, 221773
AppSec [baseline] (30.273 ms) : 0, 30273
AppSec [candidate] (29.915 ms) : 0, 29915
Debugger [baseline] (6.329 ms) : 0, 6329
Debugger [candidate] (6.266 ms) : 0, 6266
Remote Config [baseline] (685.963 µs) : 0, 686
Remote Config [candidate] (671.886 µs) : 0, 672
Telemetry [baseline] (15.279 ms) : 0, 15279
Telemetry [candidate] (15.935 ms) : 0, 15935
ProfilingAgent [baseline] (109.579 ms) : 0, 109579
ProfilingAgent [candidate] (107.819 ms) : 0, 107819
Profiling [baseline] (110.223 ms) : 0, 110223
Profiling [candidate] (108.463 ms) : 0, 108463
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 11 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section baseline
no_agent (37.02 ms) : 36721, 37319
. : milestone, 37020,
appsec (47.332 ms) : 46903, 47761
. : milestone, 47332,
code_origins (45.358 ms) : 44957, 45760
. : milestone, 45358,
iast (45.227 ms) : 44833, 45621
. : milestone, 45227,
profiling (48.142 ms) : 47712, 48571
. : milestone, 48142,
tracing (42.636 ms) : 42270, 43001
. : milestone, 42636,
section candidate
no_agent (37.377 ms) : 37075, 37678
. : milestone, 37377,
appsec (46.571 ms) : 46161, 46982
. : milestone, 46571,
code_origins (45.823 ms) : 45419, 46228
. : milestone, 45823,
iast (44.924 ms) : 44533, 45316
. : milestone, 44924,
profiling (47.884 ms) : 47457, 48311
. : milestone, 47884,
tracing (43.577 ms) : 43182, 43973
. : milestone, 43577,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section baseline
no_agent (4.386 ms) : 4335, 4437
. : milestone, 4386,
iast (9.168 ms) : 9020, 9316
. : milestone, 9168,
iast_FULL (14.311 ms) : 14025, 14598
. : milestone, 14311,
iast_GLOBAL (9.926 ms) : 9755, 10096
. : milestone, 9926,
profiling (8.844 ms) : 8712, 8977
. : milestone, 8844,
tracing (7.777 ms) : 7659, 7896
. : milestone, 7777,
section candidate
no_agent (4.366 ms) : 4315, 4417
. : milestone, 4366,
iast (9.448 ms) : 9287, 9610
. : milestone, 9448,
iast_FULL (14.802 ms) : 14506, 15098
. : milestone, 14802,
iast_GLOBAL (10.496 ms) : 10314, 10678
. : milestone, 10496,
profiling (8.718 ms) : 8577, 8859
. : milestone, 8718,
tracing (7.643 ms) : 7536, 7749
. : milestone, 7643,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section baseline
no_agent (1.477 ms) : 1466, 1489
. : milestone, 1477,
appsec (3.646 ms) : 3427, 3864
. : milestone, 3646,
iast (2.213 ms) : 2151, 2276
. : milestone, 2213,
iast_GLOBAL (2.251 ms) : 2187, 2314
. : milestone, 2251,
profiling (2.073 ms) : 2020, 2125
. : milestone, 2073,
tracing (2.019 ms) : 1970, 2067
. : milestone, 2019,
section candidate
no_agent (1.474 ms) : 1463, 1486
. : milestone, 1474,
appsec (3.59 ms) : 3378, 3803
. : milestone, 3590,
iast (2.206 ms) : 2144, 2269
. : milestone, 2206,
iast_GLOBAL (2.233 ms) : 2170, 2295
. : milestone, 2233,
profiling (2.059 ms) : 2007, 2111
. : milestone, 2059,
tracing (2.017 ms) : 1969, 2065
. : milestone, 2017,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~16d5f36836, baseline=1.53.0-SNAPSHOT~65b2349214
dateFormat X
axisFormat %s
section baseline
no_agent (14.834 s) : 14834000, 14834000
. : milestone, 14834000,
appsec (14.877 s) : 14877000, 14877000
. : milestone, 14877000,
iast (18.314 s) : 18314000, 18314000
. : milestone, 18314000,
iast_GLOBAL (18.021 s) : 18021000, 18021000
. : milestone, 18021000,
profiling (15.674 s) : 15674000, 15674000
. : milestone, 15674000,
tracing (14.871 s) : 14871000, 14871000
. : milestone, 14871000,
section candidate
no_agent (15.608 s) : 15608000, 15608000
. : milestone, 15608000,
appsec (14.78 s) : 14780000, 14780000
. : milestone, 14780000,
iast (18.089 s) : 18089000, 18089000
. : milestone, 18089000,
iast_GLOBAL (17.985 s) : 17985000, 17985000
. : milestone, 17985000,
profiling (15.345 s) : 15345000, 15345000
. : milestone, 15345000,
tracing (14.922 s) : 14922000, 14922000
. : milestone, 14922000,
|
@@ -26,12 +26,34 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) { | |||
} | |||
|
|||
public Rule(Object yaml) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if this constructor is too hard to read and if you recommend I move the "require fields" logic into helper functions (in a utils class to be used by both Rule and Selector?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I would rather try to bring type safety to the Java API as soon as possible even if SnakeYaml is returning loose Object
.
An idiomatic Java would be to have a static builder like static Rule from(Map<?, ?> rules)
and check that its parameters is the right time before calling it.
-- Note that a better type for rules
would be Map<String, ?>
but there is no way to enforce / test it due to type erasure in Java.
So far example, I would check the type ahead in StableConfig
before trying to create rule using its fromMap
:
this.apmConfigurationRules = parseRules(yaml);
// with some helper methods in StableConfig
private List<Rule> parseRules(Map<?, ?> yaml) {
Object apmConfigurationDefaultObject = map.get("apm_configuration_default");
if (apmConfigurationDefaultObject instanceof List) {
return ((List<?>) apmConfigurationDefaultObject).stream()
.filter(r -> r instanceof Map)
.map(r -> (Map<?, ?>) r)
.map(Rule::fromMap)
.collect(toList());
} else {
return emptyList();
}
}
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
@@ -26,12 +26,34 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) { | |||
} | |||
|
|||
public Rule(Object yaml) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I would rather try to bring type safety to the Java API as soon as possible even if SnakeYaml is returning loose Object
.
An idiomatic Java would be to have a static builder like static Rule from(Map<?, ?> rules)
and check that its parameters is the right time before calling it.
-- Note that a better type for rules
would be Map<String, ?>
but there is no way to enforce / test it due to type erasure in Java.
So far example, I would check the type ahead in StableConfig
before trying to create rule using its fromMap
:
this.apmConfigurationRules = parseRules(yaml);
// with some helper methods in StableConfig
private List<Rule> parseRules(Map<?, ?> yaml) {
Object apmConfigurationDefaultObject = map.get("apm_configuration_default");
if (apmConfigurationDefaultObject instanceof List) {
return ((List<?>) apmConfigurationDefaultObject).stream()
.filter(r -> r instanceof Map)
.map(r -> (Map<?, ?>) r)
.map(Rule::fromMap)
.collect(toList());
} else {
return emptyList();
}
}
return value.contains(match); | ||
default: | ||
return false; | ||
if (comparator.test(match)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Code coverage: total 57.12%, base diff 0.03%, patch 88.76% (view details) This comment will be updated automatically if new data arrives.🔗 Commit SHA: 16d5f36 | Docs | Was this helpful? Give us feedback! |
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
One question about testing logging though
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
...al-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow up PR and the improvements 👍
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
...nal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java
Outdated
Show resolved
Hide resolved
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
…ider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <[email protected]>
…ider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <[email protected]>
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
...nal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
} else if (log.isDebugEnabled()) { | ||
log.error("Unexpected error while reading stable configuration file {}: {}", filePath, e); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this if (log.isDebugEnabled())
...
To me it looks like we can log that error always, because all major cases were processed in previous if
block.
Also, I feel that here we have a very tricky case: you have 2 placeholders {}
, but the second argument e
is actually an exception and it will be treaded in a special way. So you should use only one place holder, or provide as second argument something else, for example e.getMessage()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the log.isDebugEnabled()
check: this was something @mcculls suggested; we want to print the complete exception if and only if debug is enabled, else just print the exception message. An alternative is to only print the message regardless, I suppose. WDYT?
As for the placeholder: I was not aware, thanks for sharing. 🤔 I will change this accordingly, based on what we decide on the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it! make sense in debug to print stack trace
but code should be with one place holder:
log.error("Unexpected error while reading stable configuration file: {}", filePath, e);
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
…ider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <[email protected]>
…-java into mtoff/scfg-improvements
} else if (log.isDebugEnabled()) { | ||
log.error("Unexpected error while reading stable configuration file {}: {}", filePath, e); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it! make sense in debug to print stack trace
but code should be with one place holder:
log.error("Unexpected error while reading stable configuration file: {}", filePath, e);
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
…ider/StableConfigSource.java Co-authored-by: Alexey Kuznetsov <[email protected]>
…-java into mtoff/scfg-improvements
What Does This Do
matchOperator
logic to use method reference comparator for efficiencyStableConfigMappingException
for better exception handling and debug logging when our custom YAML parsing fails on the StableConfig classapm_configuration_rules
is set but none of the rules match the active processRule
andSelector
classes with a static factoring functionfrom(Map<?, ?>)
to enforce type safety at the API levelMotivation
For (1): Suggestion by @PerfectSlayer in previous PR #9201
For (2) & (3): A result of design partner troubleshooting sessions
Additional Notes
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]